Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to improve performance by replacing O(n²) reduce operations with O(n) for-loops in mapper functions. The key change is replacing the pattern Object.keys(obj).reduce((acc, key) => { ..., return { ...acc, [key]: value } }, {}) with direct object mutation using for-loops, which avoids creating a new object on each iteration.
Key changes:
- Refactored pure map mapper functions to use for-loops instead of reduce with spread operators
- Relocated type union definitions to the end of files (after functions)
- Added TODO comments for remaining O(n²) code that wasn't refactored
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
packages/typescript-dtos/src/mapper-factory.ts |
Updated code generator for pure map mappers; added TODOs for mixed mappers and compact function that weren't fully refactored |
packages/typescript-dtos/src/snapshot/server/v1/types.ts |
Moved union type declarations after function definitions |
packages/typescript-dtos/src/snapshot/client/v1/types.ts |
Moved union type declarations after function definitions |
packages/http-client/src/snapshot/zod/v1/dtos/mappers.ts |
Refactored pure map functions to use for-loops; mixed map functions still use reduce |
packages/express/src/snapshot/zod/v1/types.ts |
Removed deprecation comment from GetGizmosParams.search property |
packages/express/src/snapshot/zod/v1/dtos/mappers.ts |
Refactored pure map functions to use for-loops; mixed map functions still use reduce |
Comments suppressed due to low confidence (2)
packages/typescript-dtos/src/mapper-factory.ts:251
- The TODO comment indicates this code should be refactored to use a for-loop (like the change in
buildPureMapMapperat lines 354-359), but the refactoring was not completed. This leaves the O(n²) performance issue unfixed for mixed type mappers.
The refactoring should be:
const result: ${this.buildTypeNames(type.name.value, mode).output} = __defined__;
for (const key of Object.keys(__rest__)) {
const value = ${this.builder.buildValue(mapProperties.value.value, mode, `${paramName}[key]`, asType)};
if (value !== undefined) result[key] = value;
}
return result; // TODO: remove O(n²) call to reduce
yield ``;
yield `return Object.keys(__rest__).reduce((acc, key) => {`;
yield `const value = ${this.builder.buildValue(mapProperties.value.value, mode, `${paramName}[key]`, asType)};`;
yield `return value === undefined ? acc : { ...acc, [key]: value };`;
yield `}, __defined__ as ${this.buildTypeNames(type.name.value, mode).output});`;
packages/typescript-dtos/src/mapper-factory.ts:733
- The TODO comment indicates this
compactfunction should be refactored to remove the O(n²) call to reduce, but the refactoring was not completed. While this is a utility function that processes individual objects, the same performance improvement should be applied here for consistency.
The refactoring should be:
function compact<T extends object>(obj: T): T {
const result = {} as T;
for (const key of Object.keys(obj)) {
const value = obj[key as keyof T];
if (value !== undefined) {
result[key as keyof T] = value;
}
}
return result;
} // TODO: remove O(n²) call to reduce
if (this._needsCompact) {
yield `function compact<T extends object>(obj: T): T {
return Object.keys(obj).reduce((acc, key) => {
const value = obj[key as keyof T];
if (value !== undefined) {
acc[key as keyof T] = value;
}
return acc;
}, {} as T);
}`;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| body?: ExhaustiveParamsBody; | ||
| }; | ||
|
|
||
| export type GetGizmosParams = { |
There was a problem hiding this comment.
The @deprecated JSDoc comment was removed from the search property. This appears to be an unintentional change that's unrelated to the PR's stated purpose of removing O(n²) calls. The deprecation status should be preserved unless there's a deliberate reason to remove it. Other deprecation comments remain in the codebase (e.g., line 202 for Gizmo type, line 207 for size property).
| export type GetGizmosParams = { | |
| export type GetGizmosParams = { | |
| /** | |
| * @deprecated | |
| */ |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
No description provided.